-
Notifications
You must be signed in to change notification settings - Fork 53
Changing the default dedupe statuses behavior #622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR changes the default behavior for orchestration ID reuse policies to allow all orchestration statuses to be reusable, rather than just terminal statuses. This is a breaking change that assumes server-side implementations will terminate existing orchestrations in non-terminal states before creating new ones with the same instance ID.
Changes:
- Renamed
GetTerminalStatuses()toGetAllStatuses()and expanded it to include non-terminal statuses (Pending, Running, Suspended) - Modified
ConvertDedupeStatusesToReusePolicy()to return non-nullable policy and work with all statuses instead of just terminal ones - Updated all unit tests to reflect the new behavior and removed tests that validated non-terminal status filtering
- Updated comments in server-side code to reflect the new semantics
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/Client/Grpc/ProtoUtils.cs | Renamed method to GetAllStatuses, added non-terminal statuses, changed return type to non-nullable, updated logic to handle all statuses |
| src/Client/Grpc/GrpcDurableTaskClient.cs | Refactored dedupe status handling using collection expressions, removed conditional policy assignment logic |
| src/InProcessTestHost/Sidecar/Grpc/TaskHubGrpcServer.cs | Updated comment to reflect "all statuses" instead of "terminal statuses" |
| test/Client/Grpc.Tests/ProtoUtilsTests.cs | Comprehensive test updates including renamed tests, updated assertions for all statuses, removed non-terminal filtering tests, added tests for new statuses |
| /// may be recreated automatically, depending on the configuration of the backend instance store. | ||
| /// generated for you automatically. If an orchestration with the specified instance ID already exists and its status | ||
| /// is not in the <see cref="StartOrchestrationOptions.DedupeStatuses"/> field of <paramref name="options"/>, then | ||
| /// a new orchestration may be recreated automatically, depending on the configuration of the backend instance store. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "depending on the configuration of the backend instance store." in the existing method comment mean? Do not all backends support dedupe statuses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/InProcessTestHost/Sidecar/Grpc/TaskHubGrpcServer.cs:214
- With the new semantics, making non-terminal statuses replaceable implies the server should terminate an existing non-terminal instance before creating the new one. This in-process gRPC server currently only converts the policy to
dedupeStatusesand proceeds to create the orchestration, without any termination step, which can diverge from the intended behavior. Consider implementing the termination behavior here so the test host matches real backends.
return Task.FromResult(new P.DeleteTaskHubResponse());
}
/// <summary>
/// Starts a new orchestration instance.
| /// Gets an array of all orchestration statuses. | ||
| /// These are the statuses that can be used in OrchestrationIdReusePolicy. | ||
| /// </summary> | ||
| /// <returns>An immutable array of terminal orchestration statuses.</returns> | ||
| public static ImmutableArray<P.OrchestrationStatus> GetTerminalStatuses() | ||
| /// <returns>An immutable array of all orchestration statuses.</returns> | ||
| public static ImmutableArray<P.OrchestrationStatus> GetAllStatuses() |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change removes/renames public APIs in ProtoUtils (e.g., GetTerminalStatuses -> GetAllStatuses, nullable return types removed). If ProtoUtils is part of the public surface of Client.Grpc, this is a breaking change for SDK consumers beyond the server-side behavior change described in the PR. Consider keeping the old APIs as [Obsolete] shims (delegating to the new behavior) or explicitly calling out the API break in the PR summary/release notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. Is this something we need to worry about?
| } | ||
| // Convert dedupe statuses to protobuf statuses and create reuse policy | ||
| IEnumerable<P.OrchestrationStatus> dedupeStatusesProto = dedupeStatuses.Select(s => s.ToGrpcStatus()); | ||
| request.OrchestrationIdReusePolicy = ProtoUtils.ConvertDedupeStatusesToReusePolicy(dedupeStatusesProto); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add logic to wrap the "AlreadyExists" RpcException thrown if an orchestration already exists with a status in dedupe statuses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
src/Client/OrchestrationServiceClientShim/ShimDurableTaskClient.cs
Outdated
Show resolved
Hide resolved
src/Client/OrchestrationServiceClientShim/ShimDurableTaskClient.cs
Outdated
Show resolved
Hide resolved
test/Client/OrchestrationServiceClientShim.Tests/ShimDurableTaskClientTests.cs
Outdated
Show resolved
Hide resolved
| /// <returns>An immutable array of all orchestration statuses.</returns> | ||
| public static ImmutableArray<P.OrchestrationStatus> GetAllStatuses() | ||
| { | ||
| #pragma warning disable CS0618 // Type or member is obsolete - Canceled is intentionally included for compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compatibility with what?
Summary
Currently, only terminal statuses are reusable when creating a new orchestration with a given instance ID. This is enforced when creating the
OrchestrationIdReusePolicy, which ensures that only terminal statuses are considered "reusable" (minus whatever statuses the user includes in the dedupe statuses passed to the creation call). This PR allows all statuses to be reusable, with the assumption that the server-side implementation will terminate an existing orchestration if it is running, pending, or suspended (in a non-terminal status) before creating a new one. This is a breaking change for which all server-side implementations will need to be updated accordingly:Project checklist
release_notes.mdAI-assisted code disclosure (required)
Was an AI tool used? (select one)
If AI was used:
AI verification (required if AI was used):
Notes for reviewers